SOLR-16427: Enable error-prone ClassInitializationDeadlock rule #2881
Conversation
There was a problem hiding this comment.
First, thanks for working on this. It's been a problem for Lucene & Solr over the years. @uschindler , you have always been involved in this topic so I'm tagging you for any additional review.
I suggest limiting this PR to be only main, without any concern for deprecating stuff. By and large, you were deprecating very internal things.
TimeSource has annoyed me; I filed SOLR-17573 to improve that (albeit may be controversial).
| qParserPlugins.init(QParserPlugin.standardPlugins, this); | ||
| valueSourceParsers.init(ValueSourceParser.standardValueSourceParsers, this); | ||
| transformerFactories.init(TransformerFactory.defaultFactories, this); | ||
| qParserPlugins.init(QParserPlugins.standardPlugins, this); |
There was a problem hiding this comment.
These 3 registries only exist for SolrCore registration. You could move them to the same package and make them not public.
But longer term (or soon if you are to do the work), I'd rather our built-in plugins be auto-discovered and registered via embracing java.util.ServiceLoader. Then we would have no static registries. -- CC @janhoy
| /** | ||
| * @deprecated Use {@link TransformerFactories#defaultFactories} instead. | ||
| */ | ||
| @Deprecated(since = "9.8", forRemoval = true) |
There was a problem hiding this comment.
No need to deprecate this; it's way too internal. (I've worked on Solr long enough to make this judgement)
| @@ -398,7 +398,7 @@ protected ValueSource parseValueSource(int flags) throws SyntaxError { | |||
| if ((ch >= '0' && ch <= '9') || ch == '.' || ch == '+' || ch == '-') { | |||
| Number num = sp.getNumber(); | |||
| if (num instanceof Long) { | |||
| valueSource = new ValueSourceParser.LongConstValueSource(num.longValue()); | |||
| valueSource = new ValueSourceParsers.LongConstValueSource(num.longValue()); | |||
There was a problem hiding this comment.
it appears inconsistent with the lower two lines; can you static import this? Or move to top level if the below ones are (consistency)
| standardPlugins = Collections.unmodifiableMap(map); | ||
| } | ||
| @Deprecated(since = "9.8", forRemoval = true) | ||
| public static final Map<String, QParserPlugin> standardPlugins = QParserPlugins.standardPlugins; |
| return standardVSParsers.put(name, p); | ||
| } | ||
| @Deprecated(since = "9.8", forRemoval = true) | ||
| public static final Map<String, ValueSourceParser> standardValueSourceParsers = |
| * @deprecated Use {@link DocRouters#DEFAULT_NAME} instead. | ||
| */ | ||
| @Deprecated(since = "9.8", forRemoval = true) | ||
| public static final String DEFAULT_NAME = DocRouters.DEFAULT_NAME; |
There was a problem hiding this comment.
WDYT about keeping this as a constant string? Then almost nobody would have a need to reference the new DocRouters.
|
|
||
| import java.util.Collections; | ||
|
|
||
| public class MapWriters { |
There was a problem hiding this comment.
just for EMPTY feels sad...
Instead, I could imagine MapWriter.empty() that references an EMPTY constant declared on MapWriterMap
|
Thanks for the review @dsmiley. I am not familiar enough with the modified classes to know which is cnosidered internal, so your input is very valuable to me. I generally try to follow the rule "anything that is public requires at least one major version of deprecation before removing it". But since it is considered internal, I will move forward and cleanup the deprecations. :) |
https://issues.apache.org/jira/browse/SOLR-16427
Description
With the ClassInitializationDeadlock rule error-prone checks for possible class initialization deadlocks. See error-prone documentation for details about the problem this rule is fixing.
Solution
Following the suggestions of error-prone, the possible deadlocks are resolved by introducing separate classes for constant fields.
You can review commits individually for a better overview.
This PR deprecates the classes in the old location and uses the new classes project-wide for partial backwards compatibility. This may break third-party / user code that reference the old classes. If we drop the need for backwards compatibility we can remove completely the deprecated classes and methods and solely update the CHANGES.txt by mentioning the new classes.
Tests
Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.
Checklist
Please review the following and check all that apply:
mainbranch../gradlew check.